Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great start on the file-moving utility! The basic functionality is working correctly. However, there are two critical requirements that need to be addressed before this can be approved.
First, the application must only support moving files. Currently, it's possible to move directories as well. You'll need to add a check to verify that the source path points to a file. You can use fs.statSync(source).isFile() for this validation.
Second, the logic for handling destination paths needs adjustment. If a destination path ends with a /, it must be treated as a directory. If that directory does not exist, the program must throw an error. Your current implementation doesn't check for this, leading to incorrect behavior.
Once you've added these validation steps, your solution should be all set!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| return; | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
The requirements state that the app must only support moving files. You should add a check here to verify that the source path points to a file and not a directory. Consider using statSync(source).isFile() before attempting to move it.
src/app.js
Outdated
| const isDir = | ||
| existsSync(destination) && statSync(destination).isDirectory(); | ||
|
|
||
| const finDest = isDir | ||
| ? path.join(destination, path.basename(source)) | ||
| : destination; |
There was a problem hiding this comment.
This logic for determining the destination path doesn't fully meet the requirements. A destination path ending with a / must be treated as a directory, and if such a directory does not exist, an error must be thrown. Your current logic doesn't explicitly check for the trailing slash for non-existent paths.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on adding the validation to prevent moving directories! We're very close to the finish line. However, one critical requirement is not yet met, so this requires a change. The application needs to throw an error if the destination path ends with a / but the directory itself does not exist. Currently, your code renames the source file instead of reporting an error in this scenario. Please add a check before the try block in src/app.js. If a destination like someDir/ is provided and the someDir directory doesn't exist, you should log an error and exit. Once this is fixed, the functionality will be complete!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| const slicedDest = destination.endsWith('/') | ||
| ? destination.slice(0, -1) | ||
| : destination; | ||
|
|
||
| try { | ||
| const isDestDir = | ||
| existsSync(slicedDest) && statSync(slicedDest).isDirectory(); | ||
|
|
||
| const finDest = isDestDir | ||
| ? path.join(slicedDest, path.basename(source)) | ||
| : slicedDest; |
There was a problem hiding this comment.
This logic block doesn't correctly handle a key requirement: when the destination path ends with a / (like someDir/), it must be treated as a directory, and an error must be thrown if that directory does not exist.
Currently, if someDir/ is provided as a destination and the directory doesn't exist, your code proceeds to rename the source file to someDir instead of reporting an error.
You'll need to add a specific check before this try block. If destination.endsWith('/'), you should first verify that the directory path (without the trailing slash) exists. If it doesn't, you must log an error and exit, as per the task requirements.
There was a problem hiding this comment.
try {
const isDestDir =
existsSync(slicedDest) && statSync(slicedDest).isDirectory();
const finDest = isDestDir
? path.join(slicedDest, path.basename(source))
: slicedDest;
await rename(source, finDest);
console.log(`${source} was moved to ${destination}`);
} catch (e) {
console.error(The file could not be moved. Error: ${e});
} - у нас программа попадет в ветку catch в этом случае если finDest не существует, отдельно это обрабатывать не нужно
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving this submission as an educational override to help you progress, but there is one critical requirement from the previous review that still needs to be addressed. You've done a great job implementing the core logic for moving and renaming files!
The main issue is that the application incorrectly handles destination paths that end with a / but do not exist. Instead of throwing an error as required, your code renames the source file to match the destination directory name.
To fix this, please add a check before the try...catch block. If destination.endsWith('/') is true, you must verify that the directory (the path without the trailing slash) actually exists. If it doesn't, you should report an error and exit. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| const slicedDest = destination.endsWith('/') | ||
| ? destination.slice(0, -1) | ||
| : destination; | ||
|
|
||
| try { | ||
| const isDestDir = | ||
| existsSync(slicedDest) && statSync(slicedDest).isDirectory(); | ||
|
|
||
| const finDest = isDestDir | ||
| ? path.join(slicedDest, path.basename(source)) | ||
| : slicedDest; | ||
|
|
||
| await rename(source, finDest); | ||
| console.log(`${source} was moved to ${destination}`); | ||
| } catch (e) { | ||
| console.error(`The file could not be moved. Error: ${e}`); | ||
| } |
There was a problem hiding this comment.
This logic still doesn't meet the requirement for handling destination paths that end with a /. According to the task description: "In case destination directory does not exist an error must be thrown."
Currently, if the destination is nonExistentDir/, your code removes the trailing slash, sees that nonExistentDir doesn't exist, and proceeds to rename the source file to nonExistentDir. This is incorrect.
You need to add a specific check before this try block. If destination.endsWith('/') is true, you must verify that the path (without the /) points to an existing directory. If it does not, you should log an error and exit.
2 participants
No description provided.